Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Init custom translations #8790

Merged
merged 11 commits into from
Apr 12, 2023
Merged

Init custom translations #8790

merged 11 commits into from
Apr 12, 2023

Conversation

AlexAndBear
Copy link
Contributor

@AlexAndBear AlexAndBear commented Apr 11, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@update-docs
Copy link

update-docs bot commented Apr 11, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@AlexAndBear AlexAndBear marked this pull request as ready for review April 11, 2023 14:07
@@ -60,8 +62,31 @@ export const bootstrapApp = async (configurationPath: string): Promise<void> =>
})
const themePromise = announceTheme({ store, app, designSystem, runtimeConfiguration })
await Promise.all([applicationsPromise, themePromise])
const customTranslations = {}

for (const customTranslation of configurationManager.customTranslations) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move fetching and collecting/merging the custom translations into its own method? Something like fetchCustomTranslations which then returns the customTranslations object. Otherwise this is an abstraction level mismatch. All the other code in this bootstrapApp method is on a higher abstraction level.

@kulmann
Copy link
Member

kulmann commented Apr 11, 2023

I already went ahead and added customTranslations to the web config struct in ocis, see owncloud/ocis#6032

I needed the scripts and styles keys, so adding customTranslations in the same PR was a low hanging fruit... hope that's fine for you.

@@ -60,8 +63,13 @@ export const bootstrapApp = async (configurationPath: string): Promise<void> =>
})
const themePromise = announceTheme({ store, app, designSystem, runtimeConfiguration })
await Promise.all([applicationsPromise, themePromise])
const customTranslations = await loadCustomTranslations({ configurationManager })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last nitpick: tiny performance improvement possible by splitting this into promise creation and awaiting the resolved promise together with other promises. This is a good thing because the promise execution starts immediately with creation, so this enables parallel processing instead of awaiting promises sequentially.

In other words:
Please split this line into:

const customTranslationsPromise = loadCustomTranslations({ configurationManager })

for promise creation and add that to the Promise.all. You can access the resolved custom translations like this:

const [customTranslations] = await Promise.all([customTranslationsPromise, applicationsPromise, themePromise])

@kulmann
Copy link
Member

kulmann commented Apr 12, 2023

Linter complains :-)

@kulmann
Copy link
Member

kulmann commented Apr 12, 2023

Screenshot 2023-04-12 at 13 31 27

Great value for any oCIS admin out there 🚀

@sonarcloud
Copy link

sonarcloud bot commented Apr 12, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@AlexAndBear AlexAndBear merged commit a64286c into master Apr 12, 2023
@delete-merged-branch delete-merged-branch bot deleted the custom-translations branch April 12, 2023 15:11
ownclouders pushed a commit that referenced this pull request Apr 12, 2023
@micbar micbar mentioned this pull request May 3, 2023
89 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[web] translation override
2 participants